-
Notifications
You must be signed in to change notification settings - Fork 220
Better patterns for sharing headers/data #1411
Conversation
1578b1b
to
5eb47e5
Compare
9580732
to
c71e2f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple notes about the README not matching the API, but otherwise this looks great.
gems/quilt_rails/README.md
Outdated
@@ -326,67 +326,42 @@ function App() { | |||
export default App; | |||
``` | |||
|
|||
##### Example: sending headers from Rails controller | |||
##### Example: Sending headers and custom data from Rails controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these maybe be two different sections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, they should. Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried separating these. Lots of redundancy so I just kept them together.
gems/quilt_rails/README.md
Outdated
|
||
```ruby | ||
class ReactController < ApplicationController | ||
include Quilt::ReactRenderable | ||
|
||
def index | ||
render_react(headers: { 'x-custom-header': 'header-value-a' }) | ||
render_react(data: { 'x-custom-header': 'header-value-a', 'some_id': 123 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think x-custom-header
should be passed into data
, should it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it shouldn't. This is wrong. In this case, the x-custom-header
header would be hidden behind the x-quilt-data
header which is not what we'd want for a header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support the separation of headers/data while ensuring it's easily accessible on the client, I've merged the headers and data in the x-quilt-data
response header to the react-server.
@@ -6,29 +6,29 @@ module Quilt | |||
module ReactRenderable | |||
include ReverseProxy::Controller | |||
|
|||
def render_react(headers: {}) | |||
def render_react(headers: {}, data: {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the example above should be using headers
as well as data
d6f3bc3
to
5d6cc91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look, but nothing to add.
5d6cc91
to
bc5b3fd
Compare
Description
Closes #1400
This PR improves the way we add basic data and headers to the React server via Rails. This approach essentially follows whats documented in #1400
Checklist